Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Local Network Access support #833

Merged

Conversation

jjurgens0
Copy link
Contributor

@jjurgens0 jjurgens0 commented Feb 14, 2023

This PR adds support for private network access. The feature was already added in PR #745, but unfortunately it hasn’t been merged yet. This PR builds on #745 but addresses the review comments in the original PR.
Details about private network access can be found here on the Chrome blog. The original release plan is to enforce private network access in version 111 (at the earliest), which is the next version to be released.

According to the implementation status, the feature might land in Chrome 113.

@jjurgens0 jjurgens0 force-pushed the jjurgens/feature/private-network-access branch from 79e89e7 to 28f6c9c Compare February 14, 2023 13:00
@untidy-hair

This comment was marked as spam.

@jjurgens0 jjurgens0 force-pushed the jjurgens/feature/private-network-access branch from 28f6c9c to 178e917 Compare February 27, 2023 08:41
@jjurgens0
Copy link
Contributor Author

Hey @adamchainz 👋 Did you have a chance to take a look at this PR yet? Anything I can improve or change?

@untidy-hair

This comment was marked as spam.

jjurgens0 and others added 5 commits May 12, 2023 09:46
This commit adds support for configuring private network access. It
allows configuring whether a Django instance residing on an IP from
the private IP range can be accessed by a browser enforcing private
network access if the CORS origin server resides on a public IP.
* Added config variable CORS_ALLOW_PRIVATE_NETWORK_ACCESS
* Added code to middleware to send appropriate header if the
  browser asks for it
* Added tests for middleware and config
* Updated README and added a short info to the changelog
Changed test to different origin to make it clearer
* Removed unused import
* Marked unreachable test code
Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updated PR @jjurgens0 . I didn't prioritize reviewing it because it seems like a niche feature that's still in draft but I'm updating and merging it now. I'll address the small comments I've made shortly.

@untidy-hair I don't appreciate comments like "Can you expedite this?" or "Any update?". If you want to help move a PR forwards, review it or add some information like "I have deployed this successfully and it passes my tests".

README.rst Outdated
@@ -304,6 +304,18 @@ Change the setting to ``'None'`` if you need to bypass this security restriction

.. _SESSION_COOKIE_SAMESITE: https://docs.djangoproject.com/en/stable/ref/settings/#std:setting-SESSION_COOKIE_SAMESITE

``CORS_ALLOW_PRIVATE_NETWORK_ACCESS: bool``
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the setting name is quite wordy, let's cut the word access which isn't really adding any info and keeps the setting like the header says "allow private network"

README.rst Outdated
@@ -304,6 +304,18 @@ Change the setting to ``'None'`` if you need to bypass this security restriction

.. _SESSION_COOKIE_SAMESITE: https://docs.djangoproject.com/en/stable/ref/settings/#std:setting-SESSION_COOKIE_SAMESITE

``CORS_ALLOW_PRIVATE_NETWORK_ACCESS: bool``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

underline must match title length in reStructuredText

README.rst Outdated
Comment on lines 310 to 312
It set to ``True``, browsers running in CORS mode and enforcing private network access will be
still be allowed to access this Django instance if it is running on an IP from the private IP
address space while the CORS origin's server is running on an IP from the public IP address space.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wordy sentence

CHANGELOG.rst Outdated
@@ -2,6 +2,9 @@
Changelog
=========

* Added support for `private network access <https://wicg.github.io/local-network-access/>`_.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's mention the setting in the changelog

@adamchainz
Copy link
Owner

Hmmm.. there's a huge debate about the name "private" versus "local": WICG/private-network-access#91 . I guess we'll ship with "PRIVATE" and support it as an alias if necessary, but still annoying how preliminary the standards are here.

@adamchainz adamchainz force-pushed the jjurgens/feature/private-network-access branch from 178e917 to 16255d9 Compare May 12, 2023 09:09
@adamchainz adamchainz changed the title Adds support for private network access Add Private Network Access support May 12, 2023
@adamchainz adamchainz changed the title Add Private Network Access support Add Local Network Access support May 12, 2023
@adamchainz adamchainz merged commit 71557f7 into adamchainz:main May 12, 2023
@untidy-hair
Copy link

@adamchainz My apologies for having bothered you with my comments. I will keep it in mind and be more careful in the future. Thank you for the review and the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants